Skip to content

Conversation

@dwightwatson
Copy link
Collaborator

This updates the channel to dispatch additional Laravel events, providing more hooks to get the FCM responses in apps.

  • Illuminate\Notifications\Events\NotificationSending
  • Illuminate\Notifications\Events\NotificationSent

I looked to the Laravel channels for Vonage and Slack for inspiration and was surprised to see they didn't fire these events. So I've inserted them where I think it seems appropriate.

@dwightwatson dwightwatson merged commit 2685d4f into master Nov 13, 2024
6 of 7 checks passed
@dwightwatson dwightwatson deleted the events branch November 13, 2024 22:05
@gdebrauwer
Copy link
Contributor

@dwightwatson These changes cause a breaking change! Before these changes, the framework automatically dispatched the NotificationSent event using the collection of MulticastSendReport objects returned by the send method of the FcmChannel class. With the changes you made, the NotificationSent event is now fired with a SendReport and that multiple times if there are multiple SendReport objects.

Can these changes be reverted? If they need to stay, then a new major version should be tagged. In my opinion, the NotificationSent event should not be fired by the package. Let the framework handle that automatically. It is also weird that multiple NotificationSent events would be triggered when there are multiple SendReport objects in a MulticastSendReport while you only send a single notification class from within your application code.

@dwightwatson
Copy link
Collaborator Author

Thanks and apologies @gdebrauwer. I couldn't understand why I wasn't familiar with these events before, I did not realise they were fired by the framework instead.

I have reverted these changes and will tag the next release to cover them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants